fix: correct SnapTrade cash activity signs#1634
Conversation
brin-security-scanner
Bot
added
the
contributor:flagged
|
| Dimension | Score | What it measures |
|---|---|---|
| Identity | 45 | Account age, contribution history, GPG keys, org memberships |
| Behavior | 90 | PR patterns, unsolicited contribution ratio, activity cadence |
| Content | 100 | PR body substance, issue linkage, contribution quality |
| Graph | 30 | Cross-repo trust, co-contributor relationships |
Analyzed by Brin · Full profile
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 6/8 reviews remaining, refill in 7 minutes and 54 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/models/snaptrade_account/activities_processor.rb (1)
247-256:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGeneric
TRANSFERtype is not normalized bynormalize_cash_amount.
TRANSFERis present inCASH_TYPES(line 45) and maps to"Transfer"in the label table, but it falls through toelse amountinnormalize_cash_amount. SnapTrade's sign convention is that a positive amount represents gaining cash and a negative amount represents spending cash. If a broker emits a bareTRANSFERwith a positive amount (cash received), it will be stored as positive in Sure—the wrong sign for an inflow. This is the same class of bug being fixed forTRANSFER_IN/TRANSFER_OUT.🐛 Proposed fix
when "CONTRIBUTION", "TRANSFER_IN", "DIVIDEND", "DIV", "INTEREST", "CASH" - -amount.abs # Money in should be negative in Sure + -amount.abs # Money in should be negative in Sure + when "TRANSFER" + # Direction is ambiguous; normalize using SnapTrade's own sign: + # positive = inflow → negate; negative = outflow → negate to positive + -amountAlternatively, if
TRANSFERis always directional via its sign, simply negate it:+ when "TRANSFER" + -amount🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/snaptrade_account/activities_processor.rb` around lines 247 - 256, The normalize_cash_amount method fails to handle the generic "TRANSFER" type; update normalize_cash_amount (in app/models/snaptrade_account/activities_processor.rb) to treat "TRANSFER" the same as "TRANSFER_IN" by returning -amount.abs for inflows (so add "TRANSFER" to the when branch that currently lists "CONTRIBUTION", "TRANSFER_IN", "DIVIDEND", "DIV", "INTEREST", "CASH"); this ensures bare TRANSFER values are normalized to the correct sign in Sure.test/models/snaptrade_account/activities_processor_test.rb (1)
115-132:⚠️ Potential issue | 🟠 MajorStale assertion in
test/models/snaptrade_account_processor_test.rbwill break CI.The test at line 213
"activities processor normalizes withdrawal as negative amount"contains an assertion at line 231 (assert tx_entry.amount.negative?) that contradicts the PR's new normalization behavior. After this change,WITHDRAWALis normalized toamount.abs(positive), causing the assertion to fail.Fix by updating the test name and assertion:
Required changes
- test "activities processor normalizes withdrawal as negative amount" do + test "activities processor normalizes withdrawal as positive outflow amount" do ... - assert tx_entry.amount.negative? + assert tx_entry.amount.positive? end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/snaptrade_account/activities_processor_test.rb` around lines 115 - 132, Update the test that still asserts a withdrawal is negative: rename the test description from the old "normalizes withdrawal as negative amount" to reflect the new normalization (e.g., "normalizes withdrawal as positive amount") and replace the failing assertion `assert tx_entry.amount.negative?` with an assertion that the amount is positive/absolute (for example, `assert_equal tx_entry.amount, tx_entry.amount.abs` or `assert tx_entry.amount > 0`) so it matches the new normalization behavior in SnaptradeAccount::ActivitiesProcessor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/models/snaptrade_account/activities_processor.rb`:
- Around line 247-256: The normalize_cash_amount method fails to handle the
generic "TRANSFER" type; update normalize_cash_amount (in
app/models/snaptrade_account/activities_processor.rb) to treat "TRANSFER" the
same as "TRANSFER_IN" by returning -amount.abs for inflows (so add "TRANSFER" to
the when branch that currently lists "CONTRIBUTION", "TRANSFER_IN", "DIVIDEND",
"DIV", "INTEREST", "CASH"); this ensures bare TRANSFER values are normalized to
the correct sign in Sure.
In `@test/models/snaptrade_account/activities_processor_test.rb`:
- Around line 115-132: Update the test that still asserts a withdrawal is
negative: rename the test description from the old "normalizes withdrawal as
negative amount" to reflect the new normalization (e.g., "normalizes withdrawal
as positive amount") and replace the failing assertion `assert
tx_entry.amount.negative?` with an assertion that the amount is
positive/absolute (for example, `assert_equal tx_entry.amount,
tx_entry.amount.abs` or `assert tx_entry.amount > 0`) so it matches the new
normalization behavior in SnaptradeAccount::ActivitiesProcessor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7cb5bd6-10f4-45fb-a150-c27ad301585a
📒 Files selected for processing (2)
app/models/snaptrade_account/activities_processor.rbtest/models/snaptrade_account/activities_processor_test.rb
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/models/snaptrade_account_processor_test.rb (1)
231-231: ⚡ Quick winUse exact decimal assertion for monetary values
At Line 231, converting to
Floatcan mask precision issues in money tests. Assert the decimal value directly instead.Proposed change
- assert_equal 1000.00, tx_entry.amount.to_f + assert_equal BigDecimal("1000.00"), tx_entry.amount🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/snaptrade_account_processor_test.rb` at line 231, Replace the float conversion assertion that masks precision—change the assertion that currently calls assert_equal 1000.00, tx_entry.amount.to_f to assert the exact decimal value instead (e.g., compare against BigDecimal("1000.00") or the model's money type directly). Update the assertion to use tx_entry.amount (or tx_entry.amount.to_d) and compare to a BigDecimal literal so the test checks exact monetary precision.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/models/snaptrade_account_processor_test.rb`:
- Line 231: Replace the float conversion assertion that masks precision—change
the assertion that currently calls assert_equal 1000.00, tx_entry.amount.to_f to
assert the exact decimal value instead (e.g., compare against
BigDecimal("1000.00") or the model's money type directly). Update the assertion
to use tx_entry.amount (or tx_entry.amount.to_d) and compare to a BigDecimal
literal so the test checks exact monetary precision.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44ce7b8c-99f7-455c-ae28-b4253e783321
📒 Files selected for processing (1)
test/models/snaptrade_account_processor_test.rb
jjmata
removed
contributor:flagged
github-project-automation
Bot
moved this to Todo
in Stable release train (v0.7.0) - April 2026
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
2 participants
Summary
Closes #1242
Details
Sure uses:
SnapTrade cash activity normalization was inverted for dividends and other cash movements. This patch corrects:
DIVIDEND,DIV,CONTRIBUTION,TRANSFER_IN,INTEREST,CASH-> negativeWITHDRAWAL,TRANSFER_OUT,FEE,TAX-> positiveTesting
Summary by CodeRabbit